Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement access control for AccountStore members #3204

Open
wants to merge 31 commits into
base: dev
Choose a base branch
from

Conversation

silva-fj
Copy link
Contributor

@silva-fj silva-fj commented Dec 13, 2024

As topic, this PR introduces an access control mechanism for the OmniAccount (AccountStore) members.

@silva-fj silva-fj requested review from kziemianek, Kailai-Wang and a team December 13, 2024 17:06
Copy link

linear bot commented Dec 13, 2024

@silva-fj silva-fj force-pushed the p-1240-implement-access-control-for-accountstore-members-in-the branch from d9a4b41 to 8cabe70 Compare December 16, 2024 09:34
return Err(Error::<T>::NoPermission);
}
},
_ => return Ok(()),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering: is it better to return error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning Err means the member account does not have permissions, but that's not the case if it reaches this point.

The permissions for the call are checked at the beginning of the method, the extra checks for add_account and set_permissions are to make sure the caller cannot set permissions or add an account with more permissions than it (the caller) has. For all the other calls the permissions have already been checked.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I ask is: ensure_permission is only called by add_account and set_permissions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ensure_permissions is called on dispatch_as_signed and dispatch_as_omni_account (in this case the call to dispatch could be add_account or set_permissions for example)

Copy link
Collaborator

@BillyWooo BillyWooo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, LGTM. Left one comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants